Skip to content

fix: 7 reported findings - #2578

Merged
ethicnology merged 8 commits into
developfrom
security-audit
Aug 10, 2026
Merged

fix: 7 reported findings#2578
ethicnology merged 8 commits into
developfrom
security-audit

Conversation

@ethicnology

@ethicnology ethicnology commented Aug 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Responds to the external static-audit report of 7 findings against 8b64b17f1b86

Every finding verified as real at the audited commit. Each was demonstrated with a failing-behavior reproducer run against develop (scratch tests asserting the vulnerable behavior, run in a throwaway worktree), then fixed with a regression test asserting the secure behavior. One independently revertable commit per finding, plus one integration-fixture isolation commit required to keep the funded Payjoin test deterministic in the aggregated CI suite.

Findings and fixes

| # | Finding (severity) | Verdict | Fix |
4. fix(exchange_support_chat): sanitize attachment download filenames
5. feat(all_seed_view): require PIN re-authentication to reveal seeds — gate enforced before seed reads and composed through the published AppUnlockFacade; the dependency is recorded in FEATURES.md.
6. feat(send): warn when sending to an unconfidential Liquid address — detection computed at the consumption point rather than carried on PaymentRequest, so plain-address, BIP21, direct Liquid and cross-chain swap paths are covered uniformly.
7. test(recoverbull): guard the key-server fetch HMAC fix — pins the isolated recoverbull-client-dart fix commit 92925c959f219fa4dfd0fe0f40392ddf868e2774; the formerly skipped test now runs and passes.
8. test(payjoin): isolate funded fixture environment — scopes funded-wallet setup to the skipped group, reapplies testnet immediately before the Payjoin flow, and restores both the previous Payjoin policy and mainnet afterward. This prevents coins_test from leaving the aggregated suite on mainnet and causing testnet transactions to fail as non-final on mainnet Electrum servers.

Demonstration evidence (vulnerable behavior on develop)

  • F1: refreshed and polled orders carrying a different deposit address were adopted into payment state (scratch bloc tests, green on develop).

  • make unit-test: green, including the active RecoverBull HMAC regression test.

  • make analyze: clean. dart fix --dry-run: "Nothing to fix!". make format-check and make bull-ui-check: green.

  • make checks reaches fix-check but its tee /dev/stderr pipeline is incompatible with this agent shell (No such device or address); the same gates were run individually and passed.

  • Integration: integration_test/payment_request_test.dart (incl. the F2 on-device reproducer) passes on the Linux desktop device under xvfb-run. The previous CI run exposed a shared-environment leak in the funded Payjoin fixture; commit 8 fixes it, and the post-push CI rerun is pending. The full integration_test/all_test.dart suite remains environment-blocked in this local headless VM (KeyringLocked from flutter_secure_storage).

  • Not run here: on-device Android integration (flutter test cannot attach to the Pixel from this VM — adb forwards bind on the host's loopback).

Test plan

  • make unit-test: 1004 passed, 1 skipped (the F4 guard), 0 failures.
  • make analyze: clean. dart fix --dry-run: "Nothing to fix!". make checks: green.
  • Integration: integration_test/payment_request_test.dart (incl. the F2 on-device reproducer) passes on the Linux desktop device under xvfb-run. The full integration_test/all_test.dart suite is environment-blocked in this headless VM (KeyringLocked from flutter_secure_storage — pre-existing, unrelated to these changes).
  • Not run here: on-device Android integration (flutter test cannot attach to the Pixel from this VM — adb forwards bind on the host's loopback).

Out of scope / follow-ups

  • PIN stored as plaintext in secure storage (noted in the report, not filed separately): not changed here. Against an attacker who already extracted secure storage, a fast KDF over a 4–8 digit PIN buys little; the real controls are the lockout (online) and the keystore (offline). Worth a deliberate decision, not a drive-by.
  • app_startup listener-based unlock redirect (report's low-confidence observation): not a confirmed vulnerability; a targeted widget test asserting no route other than /app-unlock builds when isPinCodeSet is a reasonable follow-up.
  • bip21URI rotation on refresh: the payjoin original PSBT always pays the pinned address regardless of the bip21 carried by a refreshed order; BIP78 checks in the payjoin library cover the rest. Flagged for awareness.
  • Process: security@bullbitcoin.com bounces for external senders (Google Group not open to posting) — this report nearly never arrived. Consider opening the group or publishing an additional channel.

@ethicnology ethicnology self-assigned this Aug 10, 2026
@claude

claude Bot commented Aug 10, 2026 •
edited
Loading

Copy link
Copy Markdown

Claude finished @ethicnology's task in 6m 33s —— View job

Reviewing PR #2578

  • Gather context (diff vs origin/develop, CLAUDE.md, changed files)
  • Review each of the 7 security fixes for correctness bugs, security issues, and regressions (3/8 review agents reported)
  • Verify candidate findings against the code
  • Post findings as inline review comments
  • Post summary

@ethicnology
fix(exchange): pin order deposit addresses across price-lock refreshes
33799b9 
The Sell/Pay confirm flow built the payin from the live order object,
and both the price-lock refresh and the periodic order poll replaced
that object unconditionally with the latest server response. A tampered
or compromised response could swap the deposit address mid-order and
silently redirect the payment; the confirm screen never showed the
address, so even a careful user had no way to notice.

Refuse to adopt a refreshed or polled order whose deposit address
differs from the one the order was created with, surface a localized
error instead, and display the deposit address on both confirm screens
so the destination is verifiable. The payin keeps targeting the
creation-time address, mirroring the pinning SwapWatcher already does
for swap claim/refund addresses.

Audit reproducers cover the refresh path, the poll path and the
confirm-time address actually handed to the PSBT build, for both Sell
and Pay.
@ethicnology
fix(app_unlock): enforce the PIN brute-force lockout
e81f9da 
The unlock "lockout" was cosmetic: AttemptUnlockWithPinCodeUsecase
always verified the PIN first and only computed a timeout afterwards
for a countdown label, nothing gated re-submission on it, and the dial
pad had no disabled state. A 4-digit PIN is 10,000 combinations with
no enforced delay between attempts.

Anchor the cooldown to a persisted wall-clock timestamp so it survives
an app restart, and refuse to even compare the PIN while it runs — a
brute-forcer gets neither a signal nor an attempt rate, whatever entry
point drives the attempts. The remaining time is what the UI counts
down (recomputing it from the attempt count restarted a full timeout
on every launch), the submit path and the dial pad are inert for the
duration, and a successful unlock clears the lockout.

Tests cover the domain gate (no verification during a cooldown, an
expired lockout lets attempts through), the timestamp persistence and
the remaining-time computation.
@ethicnology
fix(send): verify hardware-signed transaction outputs before broadcast
9061cfb 
A directly-connected Ledger or BitBox returns raw signed bytes that
updateSignedBitcoinTx stored verbatim and onConfirmTransactionClicked
broadcast unchecked, while the confirm screen kept showing the
pre-signing address and amount. A compromised device or transport
could redirect the payment or the change and the user had no way to
notice — the QR/air-gapped signer path already reviews the decoded
transaction instead.

Decode the signed transaction and compare its output set (scriptPubkey
+ amount, in order) against the unsigned PSBT the app built: signing
only adds witnesses, so any difference means tampering. A mismatch is
refused with a confirmation error, never stored for broadcast. The
usecase test runs the real bdk decoding in the host test VM (bdk_dart
ships a native-assets hook) with tampered amount, tampered script and
undecodable vectors; the cubit test pins the refuse/accept behavior.
@ethicnology
fix(exchange_support_chat): sanitize attachment download filenames
4909528 
downloadAttachment concatenated the server-supplied fileName verbatim
into a File() path, so a crafted getMessageAttachment response (a
compromised support-chat backend, a rogue agent, or a MITM) could
write attacker bytes outside the temp directory — anywhere in the
app's own sandbox, including over its local databases.

Reduce the name to its last path segment (normalizing Windows
separators too) and fall back to the opaque attachment id when nothing
usable remains; the original name is kept for the share-sheet display
label only. Tests cover the sanitizer and, end to end through the
cubit with a fake path_provider, that a traversal payload writes only
inside the temp directory.
@ethicnology
feat(all_seed_view): require PIN re-authentication to reveal seeds
2913fe6 
Opening Settings → View all seeds read every wallet's mnemonic from
secure storage as soon as the screen built — one tap away from an
unlocked app, behind only a warning dialog. The app already demands
step-up re-authentication before changing the PIN; the screen that
exposes full custody of every wallet now gets the same gate.

The cubit enforces it, not just the UI: no code path pulls raw seed
phrases out of secure storage before unlock() lands, and the screen
shows the existing PinCodeUnlockScreen until then (it succeeds
immediately when no PIN is set). Tests pin the gate: seeds are never
read pre-unlock, unlock() fetches exactly once.
@ethicnology
feat(send): warn when sending to an unconfidential Liquid address
70536a7 
A Liquid payment to an unconfidential (unblinded) address puts the
amount and asset id on-chain in the clear, and nothing in the send
flow distinguished that from a confidential address — a merchant or a
copy-paste could silently strip the payment of Liquid's privacy
properties.

Detect confidentiality structurally (blech32 HRPs lq1/tlq1, bech32
ex1/tex1, and the doubled length of a Base58Check address carrying a
blinding key) and flag the destination on the confirm screen with a
warning card. Computing it at the consumption point, rather than
carrying a field on PaymentRequest, covers the plain-address and the
liquidnetwork BIP21 paths uniformly. Tests cover the classifier, the
SendState getter, and on-device parsing of a real lwk-derived
unconfidential address.
@ethicnology
test(recoverbull): guard the pending key-server fetch HMAC fix
24d4ef3 
The pinned recoverbull-client-dart (9fd986d) decrypts the key-server's
encrypted_secret without verifying its trailing HMAC: a compromised
key server can return a tampered ciphertext that decrypts silently.
Demonstrated against the pinned client — fetchBackupKey returns a
forged 32-byte backup key whose HMAC was flipped. Impact is bounded by
the local vault decryption verifying its own HMAC downstream, but the
server-side check itself is a no-op.

The fix lives in the dependency (its telemetry branch, pulled in by
the pin bump planned after PR #2560's client merges upstream), not in
this repo. The regression test asserts the secure behavior and stays
skipped until the pin moves past the fix; SECURITY.md documents the
gap so it is not re-reported.
@ethicnology
test(payjoin): isolate funded fixture environment
d494b56
@ethicnology
ethicnology merged commit a14825c into develop Aug 10, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@ethicnology ethicnology

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@ethicnology